Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confirm content and update .asf.yaml #3453

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

StevenLuMT
Copy link
Member

@StevenLuMT StevenLuMT commented Aug 20, 2022

Motivation

Now Bookkeeper is using .asf.yaml to configure the project,
.asf.yaml details are determined

  1. Whether checks are enough,
    now these checks are required:
       contexts:
         - PR Validation
         - Backward compatibility tests
         - Bookie Tests
         - Build with macos on JDK 11
         - Build with windows on JDK 11
         - Client Tests
         - Compatibility Check Java11
         - Compatibility Check Java17
         - Compatibility Check Java8
         - Integration Tests
         - Remaining Tests
         - Replication Tests
         - StreamStorage Tests
         - TLS Tests
    
    Do we need to add others, such as: OWASP Dependency Check?

image

  1. required_approving_review_count:
    2.1) is there a need for a limit
    2.2) or is the value more reasonable, 1 or 2?

Mail talking Result

mail talking url: https://lists.apache.org/thread/o75xogn8r7zylqlwnno84dsm9nqoh5rs
this picture is the first question's conclusion

  • the current required check is sufficient, no need to add another

image

this picture is the second question's conclusion

  • If a committer sends a patch it is required only one more approval and not two

image

Changes

  1. contexts in .asf.yaml no need to update
  2. required_approving_review_count: from 2 back to 1

@zymap zymap requested a review from eolivelli August 23, 2022 00:57
@zymap zymap assigned zymap and StevenLuMT and unassigned zymap Aug 23, 2022
@hangc0276
Copy link
Contributor

Please update the PR context, thanks.

@StevenLuMT StevenLuMT changed the title .asf.yaml details are determined Confirm content and update .asf.yaml Aug 23, 2022
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

thanks

@eolivelli eolivelli merged commit 149ecd9 into apache:master Aug 23, 2022
@hangc0276 hangc0276 added this to the 4.16.0 milestone Oct 14, 2022
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
Co-authored-by: lushiji <lushiji@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants